Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor interning to properly mark memory as mutable or immutable #58351

Merged
merged 22 commits into from
Jun 19, 2019
Merged

Refactor interning to properly mark memory as mutable or immutable #58351

merged 22 commits into from
Jun 19, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 10, 2019 •
edited
Loading

r? @RalfJung

This implementation is incomplete out of multiple reasons

  • add -Zunleash_the_miri_inside_of_you tests
  • report an error if there's an UnsafeCell behind a reference in a constant
  • make validity checks actually test whether the mutability of their allocations match what they see in the type

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the double_check_const_eval branch from 7d53e06 to 11518e2 Compare February 13, 2019 15:57
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the double_check_const_eval branch 2 times, most recently from 98039de to 7e8dc45 Compare February 15, 2019 11:28
@RalfJung
Copy link
Member

op_to_const still says

// FIXME shouldn't it be the case that `mark_static_initialized` has already
// interned this?  I thought that is the entire point of that `FinishStatic` stuff?

I guess this is a good time to fix that as well?

@oli-obk oli-obk force-pushed the double_check_const_eval branch from 7e8dc45 to 99d3e10 Compare February 21, 2019 10:48
@bors
Copy link
Contributor

bors commented Feb 24, 2019

☔ The latest upstream changes (presumably #58691) made this pull request unmergeable. Please resolve the merge conflicts.

bjorn3
bjorn3 requested changes Mar 5, 2019
View reviewed changes
src/librustc_mir/const_eval.rs Outdated Show resolved Hide resolved
@oli-obk oli-obk force-pushed the double_check_const_eval branch from a4ba6bf to acc5c58 Compare March 6, 2019 13:04
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2019

I guess this is a good time to fix that as well?

done

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2019

@RalfJung I think it would make sense to merge this now and resolve the checkboxes from the OP in a separate PR.

@oli-obk oli-obk changed the title [WIP] Make interning explicitly care about types and the mutability of memory Refactor interning to properly mark memory as mutable or immutable Mar 11, 2019
@bors
Copy link
Contributor

bors commented Mar 16, 2019

☔ The latest upstream changes (presumably #59226) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 19, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 19, 2019

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Jun 19, 2019

📌 Commit 62af19b has been approved by RalfJung

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
travis_time:end:2a3a969e:start=1560931504599432808,finish=1560931506257770766,duration=1658337958
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
199536 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
156500 ./src/llvm-project/clang
150156 ./obj/build/bootstrap/debug/incremental
134688 ./obj/build/bootstrap/debug/incremental/bootstrap-1llt3ypt1ftzv
134684 ./obj/build/bootstrap/debug/incremental/bootstrap-1llt3ypt1ftzv/s-fday4gdtdx-ap62ha-2adl2f17sobif
126580 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release
123856 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps
123656 ./src/llvm-project/llvm/test/CodeGen
108532 ./src/llvm-project/lldb
---
travis_time:end:2869cc17:start=1560932515414575910,finish=1560932515419314315,duration=4738405
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:27ed481e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1bf52a60
travis_time:start:1bf52a60
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:077c4b40
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 19, 2019

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 19, 2019
@oli-obk oli-obk force-pushed the double_check_const_eval branch from fc15d7c to ce39fff Compare June 19, 2019 09:18
@RalfJung
Copy link
Member

"Was lange währt wird endlich gut"
@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2019

📌 Commit cd290c7 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2019
@bors
Copy link
Contributor

bors commented Jun 19, 2019

⌛ Testing commit cd290c7 with merge 9cb052a...

bors added a commit that referenced this pull request Jun 19, 2019
@bors
Auto merge of #58351 - oli-obk:double_check_const_eval, r=RalfJung
9cb052a 
Refactor interning to properly mark memory as mutable or immutable

r? @RalfJung

This implementation is incomplete out of multiple reasons

* [ ] add `-Zunleash_the_miri_inside_of_you` tests
* [ ] report an error if there's an `UnsafeCell` behind a reference in a constant
* [ ] make validity checks actually test whether the mutability of their allocations match what they see in the type
@bors
Copy link
Contributor

bors commented Jun 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 9cb052a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2019
@bors bors merged commit cd290c7 into rust-lang:master Jun 19, 2019
@bors bors mentioned this pull request Jun 19, 2019
Merged
@oli-obk oli-obk deleted the double_check_const_eval branch June 19, 2019 13:14
@mati865
Copy link
Contributor

mati865 commented Jun 20, 2019

Was this kind of perf regression expected?
https://perf.rust-lang.org/compare.html?start=a6cbf2d1344418cd2807cc5380ef1247647a1e12&end=9cb052acfb25c12d5e8960f9ea53b69a2f19b0e8&stat=instructions:u

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 20, 2019

partly, all the ctfe ones were definitely expected, since we are doing more work now in order to place constants into the correct memory location (before we were doing a kind of best effort thing that overaggressively placed constants into mutable memory). I'll look into improving this though.

@whitequark whitequark mentioned this pull request Jun 23, 2019
Closed
@cuviper cuviper mentioned this pull request Jun 28, 2019
Closed
@RalfJung RalfJung mentioned this pull request Aug 10, 2019
Closed
This was referenced Aug 25, 2019
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@bjorn3 bjorn3 bjorn3 requested changes

Assignees

@RalfJung RalfJung

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oli-obk @rust-highfive @bors @RalfJung @Centril @Dylan-DPC-zz @mati865 @bjorn3